Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[core] Move arguments with deleted copy ctor in TClingCallFunc #17030

Merged
merged 4 commits into from
Nov 27, 2024

Conversation

hahnjo
Copy link
Member

@hahnjo hahnjo commented Nov 25, 2024

If a type is passed by value but cannot be copy-constructed, instead move the argument. This is important to make std::unique_ptr interfaces work from Python code. Note that there are corner cases where checking for a deleted copy constructor is not sufficient; for example the STL always declares a copy constructor for std::vector<std::unique_ptr<T>>
which fails to instantiate if T is only moveable.

Closes #14425, alternative to #14426

Copy link

github-actions bot commented Nov 25, 2024

Test Results

    18 files      18 suites   4d 3h 8m 23s ⏱️
 2 683 tests  2 683 ✅ 0 💤 0 ❌
46 458 runs  46 458 ✅ 0 💤 0 ❌

Results for commit 5ba7196.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@jblomer jblomer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM for the RNTuple part.

@hahnjo hahnjo force-pushed the callfunc-move branch 2 times, most recently from 6adfb63 to c123eea Compare November 26, 2024 10:29
@hahnjo hahnjo changed the title [core] Move non-copyable arguments in TClingCallFunc [core] Move arguments with deleted copy ctor in TClingCallFunc Nov 26, 2024
hahnjo and others added 4 commits November 26, 2024 14:05
If a type is passed by value but cannot be copy-constructed, instead
move the argument. This is important to make std::unique_ptr interfaces
work from Python code. Note that there are corner cases where checking
for a deleted copy constructor is not sufficient; for example the STL
always declares a copy constructor for std::vector<std::unique_ptr<T>>
which fails to instantiate if T is only moveable.

Closes root-project#14425

Co-authored-by: Vincenzo Eduardo Padulano <[email protected]>
Copy link
Member

@vgvassilev vgvassilev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicely done, @hahnjo!

Copy link
Member

@dpiparo dpiparo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice improvement and complete test coverage: thanks!


for (auto *Ctor : RD->ctors()) {
if (Ctor->isCopyConstructor()) {
return Ctor->isDeleted();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also return true is the constructor is private or protected? (Since it has essentially the same effect)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would argue that no because it may be unexpected for the user that TClingCallFunc moves its objects when they "forget" to make their copy constructor public...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems github or I did not publish the following comment/reply:

Well I am still confused. It seems that we now get different behavior whether the user 'deletes' the copy constructor in pre-C++11 ways or post=C++1 ways:

class OldStyle {
   OldStyle(const OldStyle&); // Intentionally private and not implemented.
};
class NewStyle {
   NewStyle(const NewStyle&) = delete;
};

Making a copy constructor private is an explicit operation so I don't think consider the case '"forget" to make public' as a major concern.
So my question is why would the user would 'expects' when "TClingCallFunc to move its objects when the copy constructor is deleted" but not when the copy constructor is private. What difference is semantic do you see?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We cannot disambiguate "Intentionally private and not implemented" and forgotten, whereas delete is explicit. Users have to update their code to use C++11.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and forgotten

How can it be 'forgotten' since making it private requires intentional typing?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well we could check if there is a copy constructor that is private I think. The question is what's the 'right' behavior in this case...

@hahnjo hahnjo merged commit c8324b8 into root-project:master Nov 27, 2024
21 checks passed
@hahnjo hahnjo deleted the callfunc-move branch November 27, 2024 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

PyROOT calls into deleted copy-constructor in valid C++ scenarios
6 participants